…ption

Codex v2 review flagged clap layout bug: optional Option<String> leading
positional causes runtime panic in debug ("Found non-required positional
argument with a lower index than a required positional argument") and
parse ambiguity in release. `agentkeys store openrouter sk-xxx` is
consumed as `agent=openrouter, service=sk-xxx, key=<missing>`.

Clap offers no clean way to make a leading Option + required trailing
positionals disambiguate purely on arg count. Options considered:
- `conflicts_with` / `last = true` — doesn't help.
- Subcommand split — doubles surface, confuses users.
- Dynamic-dispatch ArgMatches — sacrifices derive macros.

Chose: keep --agent as a long flag. Preserves:
- `agentkeys store openrouter sk-xxx` (session wallet)
- `agentkeys store --agent my-bot openrouter sk-xxx` (resolve alias)

Positional form `agentkeys store 0xABC openrouter sk-xxx` is NOT preserved
from pre-#19 main — documented clearly in the long_about. Scripts can
migrate with a `store 0x -> store --agent 0x` s/// pattern.

URL-encoding fix for P2 retained.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

… wallet

Codex P2 finding on PR #18: `agentkeys revoke <my-own-wallet>` revoked
the session on the backend but left the dead token cached locally; every
subsequent command loaded the stale revoked token and failed.

cmd_revoke's `Some(target_wallet_str)` branch now case-insensitively
compares target_wallet against session.wallet. On match: call
session_store::clear_session() and return a 'was your own session — re-pair'
message (matching the no-arg self-revoke form's UX).

Tests added (19 total CLI tests, +2 from previous):
- cmd_revoke_with_own_wallet_clears_local_session — verifies the new path
  wipes the local file and emits the 'was your own session' acknowledgement.
- cmd_revoke_with_other_wallet_keeps_local_session — counterpart: revoking
  someone else's wallet must NOT touch the caller's session.

P2-2 (parallel-test HOME race) is tracked separately as issue #34 — needs
serial_test crate added cross-PR (#18, #19, #20, #27 all affected).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

P2-1 (cli): cmd_run was issuing two GET /credential/read calls when an
--env override targeted a service that auto-injection had already
fetched. Cache successful reads in a HashMap<service, ciphertext> and
reuse the cached value during --env resolution. Halves credential RTTs
for the common "auto-inject + override" flow.

P2-2 (mock): list_credentials returned 403 on cross-agent attempts
without leaving an audit trail, so probing through the new
/credential/list endpoint stayed invisible. Mirror the read_credential
contract by inserting a 'list' / 'DENIED' audit row (service_name='*')
before the 403 return. Extend integration test
list_credentials_ownership_enforced to assert the DENIED row appears.

Test: cargo test -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1
mock-server: 40 passed; cli: 18 passed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…d I/O

Two correctness issues from the second codex pass on PR #19:

1. Pre-flight --env validation ran AFTER the master-session
   list_credentials() call, so a malformed `--env NO_EQUALS` could still
   produce a backend round-trip and a list/DENIED audit row before the
   parse error fired. Move the validation loop above the
   list_credentials branch so the pre-flight guarantee in the comment
   actually holds for the master-session path.

2. The validation only checked for the presence of '=', so `--env =foo`
   (empty KEY) and `--env BAR=` (empty SERVICE) were accepted and then
   blew up later as opaque runtime errors (empty env-var name passed to
   process spawn / empty service name passed to read_credential).
   Reject both up front with a clear "KEY must not be empty" /
   "SERVICE must not be empty" message.

Tests: cmd_run_env_flag_empty_key_rejected,
cmd_run_env_flag_empty_service_rejected.

Test: cargo test -p agentkeys-cli -- --test-threads=1 → 20 passed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Per human design call on PR #20: ship the --agent flag form (option 1)
and migrate every breaking script reference in our own docs.

Files updated:
- wiki/credential-usage.md — synopsis, examples, lifecycle, env-var
  mapping section all show the new form. Added a "breaking change"
  callout at the top with the sed migration recipe. Removed the stale
  "issue #15 limitation" blockquote (issue #15 is now fixed in PR #19).
- wiki/key-security.md — the read escape-hatch + run preferred-usage
  examples updated.
- wiki/data-classification.md — credential store flow diagram updated.
- crates/agentkeys-cli/src/main.rs — top-level `long_about` (shown by
  `agentkeys --help`) now uses the --agent flag form across all examples
  and explicitly documents the omit-to-use-session-wallet behavior.
- docs/contradictions.md §4.3 — corrected the resolution narrative
  (positional Option panics clap; --agent flag is the chosen disambiguation),
  documented the breaking change + migration sed.

No code change in this commit; the actual --agent flag landed in
efa1707 already.

Test: cargo test -p agentkeys-cli -- --test-threads=1 → 19 passed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Combines the full rebase of fix/issue-16 onto main (post-#18 + #19 merges),
with all conflicts resolved in a single clean commit.

- `CredentialBackend::list_credentials` + `resolve_identity` trait methods
  live side-by-side; both `MockHttpClient` and `InProcessBackend`
  implement both.
- `cmd_run` signature is `(ctx, agent: Option<&str>, env_overrides: &[String], cmd: &[String])`:
  - `agent` from #16 (defaults to session wallet when None)
  - `env_overrides` from #19 (HashMap cache + pre-flight validation)
- `cmd_store` / `cmd_read` take `agent: Option<&str>`.
- `cmd_revoke` takes `agent: Option<&str>` (signature kept from #18).
- `main.rs` dispatches `Commands::{Store,Read,Run,Revoke} { agent, ... }`
  via `.as_deref()` since each carries `agent: Option<String>`.
- clap derive `Run` has both `--agent` flag AND `--env KEY=SERVICE` flag.
- Top-level `long_about` and all sub-command `long_about` strings show the
  new `--agent` flag form.
- `wiki/credential-usage.md` / `wiki/key-security.md` /
  `wiki/data-classification.md` migrated to the new form with a
  "breaking change" callout + sed migration recipe.
- `docs/contradictions.md` §4.3 resolution narrative corrected.
- Integration + CLI tests: 30 cli + 6 core + 46 mock-server passing.

Resolves issue #16 (wallet-optional + identity aliases) after the 3-pass
codex stalemate landed on the `--agent` flag design (human call).

Test: cargo test -p agentkeys-cli -p agentkeys-core -p agentkeys-mock-server -- --test-threads=1

…r handlers

Rebase onto main (post-#10 30-day TTL + #17 revoke + #19 credential/list)
with ENS handling and wallet-404 fixes baked in per PR #21 human design call.

## Refactor (original #13 scope)
- `handlers/identity.rs::resolve_identity_typed(db, identity_type, identity_value)`
  — single typed resolver used by `approve_auth_request` Recover branch and
  the `/identity/resolve` endpoint.
- Typed `identity_type` + `identity_value` columns on `auth_requests`.
  POST /auth-request/open now REQUIRES them for Recover and REJECTS them
  for non-Recover (400 either way).
- Modular minting: `mint_pair_session`, `mint_recover_session`,
  `mint_scope_change_session` each return a `MintOutput`. `approve_auth_request`
  is pure dispatch — no inlined per-request-type logic.

## Rebase + codex P2 fixes (new in this push)
- mint_pair_session and mint_recover_session updated to the 30-day TTL
  (2_592_000 sec) policy from #10, not the old 1-hour default.
- `resolve_identity_typed` "ens" arm added — ENS identities now resolve
  via the same identity_links lookup as alias/email (codex P2 round 1).
- `mock_client.rs` + `test_client.rs` open_auth_request Recover branches
  now serialize AgentIdentity::Ens as identity_type="ens" (was silently
  "alias").
- `resolve_identity_typed` "wallet" arm now confirms the wallet exists in
  `accounts` before returning it; unknown wallets return 404 instead of
  flowing through to a 500 on the sessions FK (codex P2 round 2).

## Tests
- 3 new regression tests:
  - `resolve_identity_ens_returns_wallet`
  - `resolve_identity_wallet_unknown_returns_not_found`
  - (+ `resolve_identity_wallet_passthrough` updated to use a real session
    wallet so the new existence check passes)
- 8 original #13 tests preserved
- cli 25 / core 6 / mock-server 56, all passing

Closes #13.

Rebase of fix/issue-12 onto current main (post-PR #18 #19 #21 #23 merges)
surfaced two concrete integration issues, fixed in this patch:

**1. Expose session_store helpers for test reuse**
- Make `agentkeys_core::session_store::fallback_path(session_id)` public
  so CLI tests can assert on the on-disk path layout.
- Re-export `agentkeys_core::session_store` at `agentkeys_cli::session_store`
  so existing test imports (`use agentkeys_cli::session_store;`) keep
  working after the old cli-local `session_store.rs` was deleted upstream.

**2. cmd_revoke clear_session calls (post-PR #18 self-revoke merge)**
PR #18 added `session_store::clear_session()` calls on self-revoke paths.
The new signature takes `session_id`, not 0 args. Pass `&ctx.session_id`
so the revoke wipes the correct namespace for any future multi-session
CLI caller.

**3. Integration tests: seed identity_links for Recover**
PR #21 tightened `resolve_identity_typed` to require the identity to
exist in `identity_links` before resolution. Two pair_tests
(`recover_full_loop`, `recover_credentials_intact`) opened Recover
requests with aliases that were never linked, so they now 404.
Added `InProcessBackend::link_identity_for_tests(identity_type,
identity_value, wallet)` that inserts directly into the mock DB's
`identity_links` table. The two failing tests now seed the alias link
before the Recover flow.

Test: cargo test -p agentkeys-core -p agentkeys-daemon -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1
core: 21; daemon: 13 + 15; cli: 30; mock-server: 56 — all passing

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Post-rebase review (claude code-reviewer) flagged 3 P1 issues that map
directly to patterns in `.github/REVIEW_GUIDELINES.md`. All fixed in
this commit plus one P2 nit that was cheap.

**P1 — URL encoding via reqwest `.query()` on scope + list_credentials**
`get_scope` and `list_credentials` in `mock_client.rs` built queries with
raw `format!()` interpolation. Wallet strings with reserved characters
(`&`, `#`, `%`, `+`, spaces) would break the request or smuggle extra
params server-side. Switched both to `.get(self.url("/path")).query(&[...])`
so reqwest percent-encodes per RFC 3986. Same pattern as the fix on PR
#20 `aa0cc95` / PR #22 `ffdc908` / PR #29 `7aa9e70`.

**P1 — Case-insensitive self-target check in `update_scope`**
The self-target reject `session.wallet_address == target_wallet` was
string-equal, but the backend stores wallets in lowercase while EIP-55
checksummed input preserves case. A master passing its own wallet in
mixed-case form would bypass the self-target guard and silently
restrict its own scope (the exact failure the guard was designed to
prevent). Switched to `eq_ignore_ascii_case` matching the pattern
established in PR #18's self-revoke detection.

**P1 — Missing DENIED audit rows on scope endpoints**
`update_scope` and `get_session_scope` returned 403 on ownership
failure without inserting an audit row, breaking the
cross-agent-probing-visibility invariant established for
`list_credentials` / `read_credential` in PR #19. Added `'scope_update'
'DENIED'` and `'scope_read' 'DENIED'` inserts before the 403 return
in both handlers.

**P2 — `--add foo --remove foo` would no-op silently**
`cmd_scope` accepted combinations like `--add X --remove X`. The add
loop would push, then `retain(!remove.contains)` would cancel, and the
backend PUT would still fire with the original scope + misleading
"Scope updated" output. Added an up-front `add ∩ remove` check that
lists overlapping services and rejects.

**P2 — Narrow UPDATE to most-recent active session**
Write-side `update_scope` UPDATEd ALL active sessions for the target
wallet; read-side `get_session_scope` always returned the scope from
the most-recent session via `ORDER BY created_at DESC LIMIT 1`.
Read/write contract mismatched on wallets with multiple active
sessions (paired + recovered). Narrowed the UPDATE to use the same
ORDER/LIMIT subquery.

Test: cargo test -p agentkeys-core -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1
core: 22 passed; cli: 35 passed; mock-server: 56 passed

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(cli): #15 part 3 (fix-15b) — agentkeys scope command

Closes part 3 of #15.

Adds `agentkeys scope <AGENT>` subcommand with `--add`/`--remove`/`--set`/`--list`
for editing a child agent's scope. Under the hood: new trait methods
`CredentialBackend::get_scope` + `update_scope` backed by a new mock-server
`PUT /session/scope` endpoint (owner-checked, updates all active sessions
for the target wallet).

Tests: 5 new CLI integration tests (list/add/remove/set/conflict). 62 total
tests across cli/core/mock-server, all green under --test-threads=1.

Minor: CommandContext::load_session made pub so integration tests can
access the current session token.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(cli/mock-server): #15b v2 — address codex P1+P2

Codex P1: update_scope handler allowed a master session to target its
own wallet, silently writing a restrictive scope_json onto the master's
session row. Subsequent credential/read calls would start failing for
services outside the accidental scope. Now rejects self-targeting with
a clear bad_request.

Codex P2 (URL encoding): cmd_scope's identity resolution built the
/identity/resolve query string via raw interpolation, breaking
aliases/emails with reserved characters ('+', '&', '=', '%', spaces).
Switched to reqwest's .query() builder which percent-encodes per RFC.

Codex P3 (--list exclusivity): --list now rejects combination with
--add/--remove/--set up front instead of silently dropping the mutation.

Deferred (tracked as follow-up): CBOR vs JSON parsing of ScopeChange
request_details in mint_scope_change_session — the scope CLI uses the
direct /session/scope endpoint, not the auth-request flow, so this
finding doesn't affect the CLI path. Will be addressed when
AuthRequest::ScopeChange approval is wired end-to-end.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(cli/mock-server): #29 v3 — claude P1+P2 review fixes after rebase

Post-rebase review (claude code-reviewer) flagged 3 P1 issues that map
directly to patterns in `.github/REVIEW_GUIDELINES.md`. All fixed in
this commit plus one P2 nit that was cheap.

**P1 — URL encoding via reqwest `.query()` on scope + list_credentials**
`get_scope` and `list_credentials` in `mock_client.rs` built queries with
raw `format!()` interpolation. Wallet strings with reserved characters
(`&`, `#`, `%`, `+`, spaces) would break the request or smuggle extra
params server-side. Switched both to `.get(self.url("/path")).query(&[...])`
so reqwest percent-encodes per RFC 3986. Same pattern as the fix on PR
#20 `aa0cc95` / PR #22 `ffdc908` / PR #29 `7aa9e70`.

**P1 — Case-insensitive self-target check in `update_scope`**
The self-target reject `session.wallet_address == target_wallet` was
string-equal, but the backend stores wallets in lowercase while EIP-55
checksummed input preserves case. A master passing its own wallet in
mixed-case form would bypass the self-target guard and silently
restrict its own scope (the exact failure the guard was designed to
prevent). Switched to `eq_ignore_ascii_case` matching the pattern
established in PR #18's self-revoke detection.

**P1 — Missing DENIED audit rows on scope endpoints**
`update_scope` and `get_session_scope` returned 403 on ownership
failure without inserting an audit row, breaking the
cross-agent-probing-visibility invariant established for
`list_credentials` / `read_credential` in PR #19. Added `'scope_update'
'DENIED'` and `'scope_read' 'DENIED'` inserts before the 403 return
in both handlers.

**P2 — `--add foo --remove foo` would no-op silently**
`cmd_scope` accepted combinations like `--add X --remove X`. The add
loop would push, then `retain(!remove.contains)` would cancel, and the
backend PUT would still fire with the original scope + misleading
"Scope updated" output. Added an up-front `add ∩ remove` check that
lists overlapping services and rejects.

**P2 — Narrow UPDATE to most-recent active session**
Write-side `update_scope` UPDATEd ALL active sessions for the target
wallet; read-side `get_session_scope` always returned the scope from
the most-recent session via `ORDER BY created_at DESC LIMIT 1`.
Read/write contract mismatched on wallets with multiple active
sessions (paired + recovered). Narrowed the UPDATE to use the same
ORDER/LIMIT subquery.

Test: cargo test -p agentkeys-core -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1
core: 22 passed; cli: 35 passed; mock-server: 56 passed

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(cli/mock-server): #29 v4 — claude re-review nits (dead sort, pct_encode consistency, test coverage)

Follow-up on the claude-code-action re-review posted against commit
57dc9fb. All 3 substantive findings addressed; the 4th (malformed `/`
doc-comments) was a false positive from pre-rebase state — grep for
`^\s*/ ` returns nothing in the current tree.

**Low — Redundant `sort_by` after `update_scope`**
`lib.rs:769` was re-sorting `new_scope.services` after the backend
call returned, but both the `--set` branch (line 749) and the
`--add`/`--remove` branch (line 760) sort before the `update_scope`
call. Dead op; removed. Added a comment so a future reader doesn't
re-add it.

**Low — `InProcessBackend::get_scope` raw URL concat**
`test_client.rs:725` built the query string via `format!()` while the
`resolve_identity` method right above uses `pct_encode`. Wallet
strings are hex so this was safe in practice, but the inconsistency
violates the URL-encoding invariant documented in
`.github/REVIEW_GUIDELINES.md` pattern #3. Swapped to `pct_encode`.

**Low — Missing test for `--list + --add` conflict**
Added `cmd_scope_list_and_add_conflict_errors`. Also added
`cmd_scope_add_remove_overlap_errors` (covering the P2 overlap guard
from v3) that wasn't yet under test.

Test: cargo test -p agentkeys-core -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1
core: 22 passed; cli: 37 passed (+2 new); mock-server: 56 passed

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>